POC of a symlink-based code sharing approach. #53417
Conversation
boring-cyborg
bot
added
area:API
|
A possible alternative to #53149 |
ashb
force-pushed
the
shared-lib-via-symlinks
branch
from
d177c2d to
ee3aecf
Compare
|
I wonder how imports between shared modules should work in this approach. |
As per Jarek here, cross-imports from shared deps would be banned:
|
|
Yep. I wil take a look tomorrow/over weekend.
This could be done as symlinks between shared modules - but I also think such imports should be discouraged. I think each shared module should be independent from each other - because that is what adspds complexithy. For example - rather than reading a configuration from logging module - you should pass a parameter from outside that provides the config. Generally spekaing - "needin a one shared module to use another shared module" is an antipattern that shold be possible when really needed, but it should be avoided. This is waht cyclomatic complexity "theory" is about when turned into practice (as far as I understand it). It you look at breeze code this is how the whole package is organized like. there are about 30 independent modules that do not use each other, whenever there is a need to share things, the shared things are moved into a "one level deep" module that is imported by other modules (global constants is a good example of it). |
|
I haven't yet looked at what this will need to make it work for sdist packages. Yeah, out of the box this won't make a "valid" sdist as it puts the symlink in the tar without any other changes: And that symlinked path isn't valid outside of the repo. This is the layout/contents of the sdist tar: So I think this means that in either approach (this or the vendoring one) we'll need a custom hatchling plugin of one form or the other. |
That is an opinion I don't agree with. Quite the oppsite in fact: I think it is very likely we will want to use the "timezone" shared lib (utcnow, date parsing) across many of the other shared libs. (Kube pod to parse logs, logging to deal with formatting are the first two that spring to mind) However "should be possible when needed" -- I don't think it is. I can't think what imports we would put in, say the logging/structlog.py code that would work in all cases (inside core, inside task-sdk and inside it's own shared tests) |
This comment was marked as resolved.
This comment was marked as resolved.
ashb
force-pushed
the
shared-lib-via-symlinks
branch
2 times, most recently
from
2b91508 to
0f20bda
Compare
And move `airflow.utils.timezone` into a shared library as the first example of it working. In this change we have now setled on an approach using symlinks, but we did explore other options (see the GH PR for discussion and previous versions, notably one built upon the `vendoring` tool) A lot of the reasoning and mode of operation of this is detailed in shared/README.md in this PR, hence why this description is so short. Currently various places in TaskSDK and Airflow Core both use these utility functions, and while in this specific case they are small enough that they could just be copied and the duplication wouldn't hurt us long term, this changes shows a way in which we can have a single source of truth, but have it included automatically in built dists. Co-authored-by: Jarek Potiuk <[email protected]>
ashb
force-pushed
the
shared-lib-via-symlinks
branch
from
0f20bda to
efbe2a2
Compare
|
Merged in to the other linked pr |
Don't read too much in to the specific paths chosen.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.